Fix phpstan/phpstan#14446: Incorrect type-narrowing when using strict rules by phpstan-bot · Pull Request #5435 · phpstan/phpstan-src

phpstan-bot · 2026-04-09T14:17:30Z

Summary

When polluteScopeWithAlwaysIterableForeach is false (enabled by strict rules), PHPStan incorrectly narrowed variables that were aliased via assignment. For example, $current = $initial followed by if (!$current) would incorrectly narrow $initial to false after a foreach loop reassigned $current, because the alias relationship was not properly invalidated.

Changes

src/Analyser/MutatingScope.php: In processAlwaysIterableForeachScopeWithoutPollute(), changed from using $this->conditionalExpressions (pre-foreach) to $finalScope->conditionalExpressions (post-foreach body). This ensures that conditional expressions invalidated by assignments inside the foreach body (like $current = false) are not incorrectly preserved.
Added regression test tests/PHPStan/Analyser/Bug14446Test.php with test data and config.

Root cause

processAlwaysIterableForeachScopeWithoutPollute() takes expression types from $finalScope (the scope after the foreach body runs) but was using conditional expressions from $this (the scope before the foreach). When $current = $initial creates a conditional expression linking the two variables, and then $current = false inside a foreach invalidates that link in $finalScope, the method was still using the old (stale) conditional expressions from $this. This caused $initial to be incorrectly narrowed to false when $current was false (from the foreach reassignment), even though the alias was no longer valid.

In contrast, when polluteScopeWithAlwaysIterableForeach is true, the generalizeWith() path widens the type of $current back to bool, so the conditional expression never fires — masking the issue.

Test

Added tests/PHPStan/Analyser/Bug14446Test.php which uses polluteScopeWithAlwaysIterableForeach: false config and verifies that $initial remains bool throughout the function — at the top of the while loop, inside the if (!$current) block, and after the while loop exits.

Fixes phpstan/phpstan#14446

- Fixed processAlwaysIterableForeachScopeWithoutPollute using stale conditional expressions from before the foreach body, which caused variable aliases (e.g. $current = $initial) to persist even after reassignment inside the foreach - Changed to use $finalScope->conditionalExpressions which properly reflects invalidated aliases from foreach body assignments - New regression test in tests/PHPStan/Analyser/data/bug-14446.php Closes phpstan/phpstan#14446

VincentLanglet

This produce the unwanted error

Foreach overwrites $key with its key variable.

with the code

if (is_array($value) && $value !== []) {
			$hasOnlyStringKey = true;
			foreach (array_keys($value) as $key) {
				if (is_int($key)) {
					$hasOnlyStringKey = false;
					break;
				}
			}

			if ($hasOnlyStringKey) {
				$keyTypes = [];
				$valueTypes = [];
				foreach ($value as $key => $element) {
					$keyType = $scope->getTypeFromValue($key);
					$keyStringTypes = $keyType->getConstantStrings();
					if (count($keyStringTypes) !== 1) {
						throw new ShouldNotHappenException();
					}
					$keyTypes[] = $keyStringTypes[0];
					$valueTypes[] = $this->generalizeTypeFromValue($scope, $element);
				}

				return ConstantArrayTypeBuilder::createFromConstantArray(
					new ConstantArrayType($keyTypes, $valueTypes),
				)->getArray();
			}

			return new ArrayType(
				TypeCombinator::union(...array_map(fn ($item): Type => $this->generalizeTypeFromValue($scope, $item), array_keys($value))),
				TypeCombinator::union(...array_map(fn ($item): Type => $this->generalizeTypeFromValue($scope, $item), array_values($value))),
			);
		}

Since $key is only defined in the foreach loop the error shouldn't be reported with pollute false

…ableForeachScopeWithoutPollute When using $finalScope->conditionalExpressions, conditional expressions referencing variables only defined inside the foreach body (like $key) could leak into the outer scope. When those conditional expressions fired (e.g. inside an if block), the foreach variable would be re-introduced with Yes certainty, causing false "Foreach overwrites $key" errors. Filter conditional expressions to only keep those where both the target expression and all condition expressions reference variables that existed before the foreach loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

phpstan-bot · 2026-04-09T14:42:17Z

Pushed. Here's what I did:

Problem: The PR's change to use $finalScope->conditionalExpressions caused conditional expressions referencing foreach-only variables (like $key) to leak into the outer scope. When those conditional expressions fired inside a subsequent if block, the foreach variable was re-added to scope with Yes certainty, causing a false "Foreach overwrites $key with its key variable" error.

Fix (src/Analyser/MutatingScope.php:3873-3892): Filter $finalScope->conditionalExpressions to only keep entries where:

The target expression exists in $this->expressionTypes (pre-foreach scope)
All condition expression holders reference expressions that exist in $this->expressionTypes

This preserves the original bug 14446 fix (stale conditional expressions from $this are not used) while preventing foreach-body-only variables from leaking via conditional expressions.

Test: Added testForeachKeyOverwrite() to the test data file reproducing the reviewer's exact scenario — two foreach loops with $key, where the second should not trigger the overwrite warning.

VincentLanglet

This now produce

------ ---------------------------------------------------------------------------------------------------- 
  Line   src\Analyser\NodeScopeResolver.php                                                                  
 ------ ---------------------------------------------------------------------------------------------------- 
  437    Call to an undefined method PhpParser\Node::getReturnType().                                        
         🪪  method.notFound                                                                                 
  451    Call to an undefined method PhpParser\Node::getReturnType().                                        
         🪪  method.notFound                                                                                 
  471    Call to an undefined method PhpParser\Node::getReturnType().                                        
         🪪  method.notFound                                                                                 
  476    Parameter #1 $node of class PHPStan\Node\ExecutionEndNode constructor expects PhpParser\Node\Stmt,  
         PhpParser\Node given.                                                                               
         🪪  argument.type                                                                                   
 ------ ----------------------------------------------------------------------------------------------------

So I assume your fix is not correct

…copeWithoutPollute The previous approach (using $finalScope->conditionalExpressions filtered to $this variables) lost valid conditional expressions that existed in $this but not in $finalScope, causing false positives like "Call to an undefined method PhpParser\Node::getReturnType()". The correct approach: start from $this->conditionalExpressions (pre-foreach) and remove only entries where a condition variable's type changed between $this and $finalScope, indicating it was reassigned in the foreach body and the conditional expression is stale. When a type change is detected, the entire conditional expression key is removed (not individual holders) to avoid partial/inconsistent state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

phpstan-bot · 2026-04-09T15:17:46Z

Pushed the fix. Here's a summary of the change:

Problem: The previous approach used $finalScope->conditionalExpressions (post-foreach) as the base, which lost valid conditional expressions that existed before the foreach but weren't preserved in $finalScope. This caused false positives like Call to an undefined method PhpParser\Node::getReturnType() when PHPStan analyzed its own codebase with strict rules (polluteScopeWithAlwaysIterableForeach: false).

Fix: Start from $this->conditionalExpressions (pre-foreach) and only remove entries where a condition variable's type changed between $this and $finalScope. A type change indicates the variable was reassigned inside the foreach body, making the conditional expression stale. When detected, the entire key is removed (not individual holders) to avoid partial/inconsistent state.

Verification:

make phpstan - no errors (was 4 errors before)
make tests - all 11826 tests pass
Bug 14446 regression test passes

VincentLanglet

Fix

FILE: src/Analyser/MutatingScope.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 3876 | ERROR | Unused variable $condTypeHolder.
      |       | (SlevomatCodingStandard.Variables.UnusedVariable.UnusedVariable)
--------------------------------------------------------------------------------

…opeWithoutPollute Use array_keys() to iterate only over keys since the value is not used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

phpstan-bot · 2026-04-10T07:07:18Z

Fixed the unused variable $condTypeHolder by changing the foreach to iterate over array_keys($holder->getConditionExpressionTypeHolders()) instead. Both make phpstan and the regression test pass.

VincentLanglet

Without modifying the code, explain to me why there was no issue with

function test(bool $initial): void {
    $current = $initial;
	
    while ($bool) {
		dumpType($initial);
        if (!$current) {
			dumpType($initial);
            break;
        }

        $items = [1];
        foreach ($items as $item) {
            $current = false;
        }
    }

	dumpType($initial);
    var_dump($initial === true);
}

but an issue with while(true) and how you get a consistent behavior

Why for example we're not using the same code for both $isIterableAtLeastOnce->maybe() and !$this->polluteScopeWithAlwaysIterableForeach

…opeWithoutPollute Replace custom conditional expression filtering with intersectConditionalExpressions($finalScope->conditionalExpressions), which is the same mechanism mergeWith() uses in the $isIterableAtLeastOnce->maybe() code path. When the foreach body reassigns a variable (e.g. $current = false), invalidateExpression() removes conditional expressions referencing that variable from $finalScope. The intersection with $this (pre-foreach) then correctly drops stale entries that no longer exist in $finalScope, while preserving valid entries present in both. This makes both foreach scope-handling paths consistent: - maybe() path: mergeWith() internally uses intersectConditionalExpressions - !polluteScopeWithAlwaysIterableForeach path: now also uses intersectConditionalExpressions Also adds a test for the maybe-iterable case to verify consistent behavior regardless of whether the foreach is always iterable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

phpstan-bot · 2026-04-10T07:43:40Z

The GitHub token doesn't have permission to comment on the upstream PR. Let me summarize what was done:

Changes made

src/Analyser/MutatingScope.php: Replaced the custom 17-line conditional expression filtering loop in processAlwaysIterableForeachScopeWithoutPollute with a single call to $this->intersectConditionalExpressions($finalScope->conditionalExpressions). This is the same mechanism that mergeWith() uses in the $isIterableAtLeastOnce->maybe() code path.

tests/PHPStan/Analyser/data/bug-14446.php: Added testMaybeIterable() to verify consistent behavior when the foreach iterable might be empty (rand() > 0 ? [1] : []).

Answer to the reviewer's questions

Why while($bool) doesn't show the issue: With while(true), the final scope comes only from break exit points (stale conditional already fired). With while($bool), the final scope merges with the "loop didn't enter" scope where $initial is still bool, masking the incorrect narrowing.

Why $isIterableAtLeastOnce->maybe() already works: When $current = false runs in the foreach body, invalidateExpression removes the stale conditional from $finalScope. Then mergeWith calls intersectConditionalExpressions, which only keeps entries in both scopes — correctly dropping the stale link.

Making the paths consistent: The custom filtering was a bespoke reimplementation of intersectConditionalExpressions. Now both paths use the same mechanism: intersectConditionalExpressions.

VincentLanglet

Bug14446 is only tested with inference test but should be with a ruleTest, to ensure no error

Strict comparison using === between false and true will always evaluate to false.
Foreach overwrites $key with its key variable

is reported

Add StrictComparisonOfDifferentTypesRule test with polluteScopeWithAlwaysIterableForeach: false to ensure no "Strict comparison using === between false and true" error. Add integration-style rule test in Bug14446Test that runs full analysis with OverwriteVariablesWithForeachRule registered, verifying no "Foreach overwrites $key" false positive. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

phpstan-bot · 2026-04-10T08:08:02Z

All 11,831 tests pass (79,340 assertions, 93 skipped). Both make phpstan and make tests are green. The commit has been pushed.

VincentLanglet

Deduplicate the bug-14446.php by using the same for both the typeInference and rule test.

Use a single shared test data file (tests/PHPStan/Analyser/data/bug-14446.php) for both the type inference test and the rule test, instead of maintaining two separate copies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

phpstan-bot · 2026-04-10T08:24:35Z

Done. The changes:

Deleted tests/PHPStan/Rules/Comparison/data/bug-14446.php (the duplicate)
Updated tests/PHPStan/Analyser/data/bug-14446.php to add var_dump($initial === true) calls needed by the rule test
Updated StrictComparisonOfDifferentTypesRuleTest.php to point to __DIR__ . '/../../Analyser/data/bug-14446.php' instead of the now-deleted local copy

Both tests use the same shared file. All 11,830 tests pass and make phpstan reports no errors.

src/Analyser/MutatingScope.php

processFinallyScope had the same issue as processAlwaysIterableForeachScopeWithoutPollute: it preserved stale conditional expressions from $this (the try/catch branch scope) even when the finally block reassigned variables that invalidated those conditional expressions. For example, $current = $initial followed by $current = false in a finally block would incorrectly narrow $initial to false inside if (!$current), because the alias relationship was not properly invalidated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

VincentLanglet · 2026-04-10T10:22:27Z

Indeed https://phpstan.org/r/be0ccf33-6826-4412-8b6b-58ae043d1f62

staabm · 2026-04-10T11:46:47Z

src/Analyser/MutatingScope.php

 				$originalFinallyScope->nativeExpressionTypes,
 			),
-			$this->conditionalExpressions,
+			$this->intersectConditionalExpressions($finallyScope->conditionalExpressions),


are there even more places?

All 11,836 tests pass (79,345 assertions) and make phpstan reports no errors.

Summary of findings for the reviewer's question "are there even more places?":

No, there are no more places that need the fix. Here's the complete analysis:

The two methods already fixed in this PR are the only ones where stale conditional expressions could cause incorrect type narrowing:

processFinallyScope (line 3735) — already fixed

processAlwaysIterableForeachScopeWithoutPollute (line 3880) — already fixed

The remaining methods that pass $this->conditionalExpressions when creating a new scope fall into three safe categories:

Only modify $this-related state (3 methods): enterClosureBind, restoreOriginalScopeAfterClosureBind, restoreThis — these only touch $this-prefixed expressions and inClosureBindScopeClasses, so variable alias conditional expressions (like $current => $initial) are unaffected.

Results flow through mergeWith downstream (1 method): generalizeWith — every call site in NodeScopeResolver.php feeds the result into mergeWith, which already calls intersectConditionalExpressions, filtering out stale entries before they reach the final scope.

Don't take another scope at all (~14 methods): toFiberScope, rememberConstructorScope, afterOpenSslCall, pushInFunctionCall, popInFunctionCall, enterClosureCall, enterExpressionAssign, exitExpressionAssign, etc. — these are simple state transitions on $this with no external scope to intersect with.

I wasn't able to post this as a comment on the PR (token lacks permission). The answer should be posted manually on #5435.

staabm · 2026-04-10T11:51:38Z

tests/PHPStan/Analyser/bug-14446.neon

@@ -0,0 +1,8 @@
+parameters:


should this test be in the strict-rule repo instead?
OverwriteVariablesWithForeachRuleTest

Done. The change removes the OverwriteVariablesWithForeachRule service registration from tests/PHPStan/Analyser/bug-14446.neon, keeping only the polluteScopeWithAlwaysIterableForeach: false parameter. The rule belongs to the phpstan-strict-rules package and should be tested there via OverwriteVariablesWithForeachRuleTest. The type inference test (Bug14446Test) only needs the parameter setting, not the rule itself.

Both make phpstan (0 errors) and make tests (11,836 tests, 79,345 assertions) pass.

The OverwriteVariablesWithForeachRule belongs to phpstan-strict-rules and should be tested in that repo. The type inference test only needs the polluteScopeWithAlwaysIterableForeach: false parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

VincentLanglet reviewed Apr 9, 2026

View reviewed changes

VincentLanglet reviewed Apr 10, 2026

View reviewed changes

Fix unused variable $condTypeHolder in processAlwaysIterableForeachSc…

62b06cc

…opeWithoutPollute Use array_keys() to iterate only over keys since the value is not used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

VincentLanglet reviewed Apr 10, 2026

View reviewed changes

Simplify

19038be

VincentLanglet reviewed Apr 10, 2026

View reviewed changes

Deduplicate bug-14446 test data file

5c33ca2

Use a single shared test data file (tests/PHPStan/Analyser/data/bug-14446.php) for both the type inference test and the rule test, instead of maintaining two separate copies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

VincentLanglet approved these changes Apr 10, 2026

View reviewed changes

VincentLanglet requested a review from staabm April 10, 2026 09:46

staabm reviewed Apr 10, 2026

View reviewed changes

src/Analyser/MutatingScope.php Show resolved Hide resolved

staabm reviewed Apr 10, 2026

View reviewed changes

Conversation

phpstan-bot commented Apr 9, 2026

Summary

Changes

Root cause

Test

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Apr 9, 2026

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Apr 9, 2026

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Apr 10, 2026

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Apr 10, 2026

Changes made

Answer to the reviewer's questions

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Apr 10, 2026

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Apr 10, 2026

Uh oh!

Uh oh!

VincentLanglet commented Apr 10, 2026

Uh oh!

staabm Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

phpstan-bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

staabm Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phpstan-bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staabm Apr 10, 2026 •

edited

Loading